[Release/7.0] Updating inbox source generators to Roslyn 4.4 and removing polyfill approach #74822

joperezr · 2022-08-30T16:29:39Z

Description

This PR is upgrading all of the 7.0 inbox source generators (Json one is pending and I will add a commit that addresses that one too in a bit) to use Roslyn version 4.4 (version picked is the roslyn included in SDK 7.0.100-rc.1.22425.9), which allows us to remove the polyfill approach from them and instead use the inbox Roslyn APIs for finding an attribute in a performant manner. Additionally, this PR is also moving the repo to use that same roslyn version for the compilers used to build the repo.

Customer Impact

After this change goes in, the only way to consume dotnet/runtime (without warnings or disabled analyzers) will be by using a Roslyn4.4 compiler. These source generators are on by default when targeting dotnet 7.0, so all projects automatically pass them in to the compiler. Since these source generators will now depend on Roslyn 4.4, then the consuming app will need to use the Roslyn 4.4 compilers too, which can be done via using any of the following:

Building using an SDK which is .NET 7 RC1 or older.
Building using Visual Studio 17.4 preview 1 or older.
Manually referencing Microsoft.NET.Compilers.Toolset package version 4.4.0-2.22423.18 or older

Regression

No.

Risk

Medium, since we are essentially changing the compilers that get used to build all of the repo, however, note that this needs to be done anyway since we want to ship .NET 7 with pretty much the final version of the compilers. This is also in a sense reducing risk, as it is removing from the inbox source generators the dependency to Roslyn API that was not fully tested and only source-copied. After this, we will now instead depend on the Roslyn tested bits.

ghost · 2022-08-30T16:29:59Z

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #70754

This PR is upgrading all of the 7.0 inbox source generators (Json one is pending and I will add a commit that addresses that one too in a bit) to use Roslyn version 4.4 (version picked is the roslyn included in SDK 7.0.100-rc.1.22425.9), which allows us to remove the polyfill approach from them and instead use the inbox Roslyn APIs for finding an attribute in a performant manner. Additionally, this PR is also moving the repo to use that same roslyn version for the compilers used to build the repo.

After the Json source generator is also ported, the only one left will be the Logging generator, which I believe only ships OOB. @eerhardt do we want to also cross compile against roslyn4.4 in that case and ship in the package? or do we want to use roslyn4.4 instead of the current 4.0? Once that is clear, I will go ahead and add to this PR both the Json source generator and Logging generator changes to this PR.

Finally, After this PR gets merged, upstack repos will be broken unless they start using this roslyn version (or greater) to build. For this reason, the plan is to, before merging this PR, merge a PR in dotnet/arcade that moves both the SDK and the Roslyn version up for all repos, and that way, after this goes in, upstack repos won't be broken.

Adding NoMerge label for now while the Json and Logging generators get ported, and the arcade changes get ready, but sending out the PR now to get early feedback.

Author:	joperezr
Assignees:	-
Labels:	`NO-MERGE`, `area-Meta`
Milestone:	-

joperezr · 2022-08-30T16:34:31Z

FYI: @jaredpar @marcpopMSFT

Directory.Build.props

...ntime.InteropServices/gen/LibraryImportGenerator/Analyzers/ConvertToLibraryImportAnalyzer.cs

src/libraries/System.Private.CoreLib/gen/System.Private.CoreLib.Generators.csproj

CyrusNajmabadi · 2022-08-30T16:44:55Z

Done with pass.

carlossanlop · 2022-08-30T17:04:44Z

@danmoseley need an approval please.

joperezr · 2022-08-30T17:17:26Z

need an approval please

Getting approval is fine now, but please do note that this is not yet ready to go in.

carlossanlop · 2022-08-30T17:34:47Z

Getting approval is fine now, but please do note that this is not yet ready to go in.

Apologies. I missed the NO MERGE label.

Directory.Build.props

danmoseley · 2022-08-30T19:45:13Z

This one is worth filling out the template for also. Eg., explaining how this is more than just code cleanup without visible effects.

. For this reason, the plan is to, before merging this PR, merge a PR in dotnet/arcade that moves both the SDK and the Roslyn version up for all repos, and that way, after this goes in, upstack repos won't be broken.

I assume we want to do that anyway, so that we ultimately ship having built with the shipping SDK, more or less.

ViktorHofer · 2022-08-30T20:42:19Z

One of the questions that Tactics might raise is why this isn't in main yet. Would it make sense to drive the required changes (including the SDK update) into main first?

joperezr · 2022-08-30T20:58:26Z

Main is already on a newer version of roslyn, that said it doesn't yet target it for the source generators specifically. I can certainly add a PR for that too.

danmoseley · 2022-08-30T21:00:58Z

Yes main first would be good - although, perhaps it would take some time before it flowed through and was testable in VS, so that would not increase coverage signficantly anytime soon (?)

carlossanlop · 2022-09-01T17:03:46Z

There's a merge conflict FYI.

marcpopMSFT · 2022-09-01T19:47:50Z

How will this work if someone is using the 7.0.100 SDK in 17.3 to target 6.0 apps? Will the source generators only be used when targeting 7.0 applications? (Reminder that we're limiting 7.0.100 to 17.3 and above and 7.0.200 will be 17.4 and up).

joperezr · 2022-09-01T20:14:52Z

the new source generators that depend on roslyn 4.4 ship as part of the 7.0 targeting pack, so they will only be referenced if you are actually targeting .NET 7.0. Even if you use the new SDK, If you are targeting net6.0 then these generators should never be passed to the compiler.

joperezr · 2022-09-01T22:15:15Z

This is now ready for review.

@danmoseley I have filled the template now in the main description.

joperezr · 2022-09-01T22:55:03Z

Once PR dotnet/arcade#10724 in arcade gets merged, all upstack repos will get an sdk bump and compiler bump automatically, which will allow them to ingest this change.

carlossanlop · 2022-09-02T16:22:26Z

Once PR dotnet/arcade#10724 in arcade gets merged, all upstack repos will get an sdk bump and compiler bump automatically, which will allow them to ingest this change.

Ping me when ready to merge, please, @joperezr .

danmoseley · 2022-09-07T19:53:10Z

Approved -- we don't want to ship the way we are, this brings us into a regular state. Of course please watch the bits after they go in to ensure we spot any unexpected issue early.

carlossanlop · 2022-09-08T22:52:47Z

There's a merge conflict. Can one of you please resolve it, @CyrusNajmabadi or @joperezr?

joperezr · 2022-09-08T22:54:37Z

Yes, I'm working on this now. Turns out that the version that got merged into Arcade of the SDK had a different problem that I only discovered, will quickly push a PR to fix that, and then will go ahead and fix the changes here and merge

…approach

joperezr · 2022-09-09T17:43:19Z

@carlossanlop conflict is resolved and this should be ready to go. Feel free to merge as soon as CI passes 👍

lewing · 2022-09-09T22:07:28Z

  Starting:    JIT.HardwareIntrinsics.XUnitWrapper (parallel test collections = on, max threads = 2)
    JIT/HardwareIntrinsics/Arm/Sha256/Sha256_r/Sha256_r.sh [FAIL]
      Unhandled exception: System.IO.IOException: Connection timed out : 'Global\msbuild-server-launch-gkQydOqVnpHlKzivWKBFxHKmWmVPqCBlomIg710sl38'
         at System.Threading.Mutex.CreateMutexCore(Boolean initiallyOwned, String name, Boolean& createdNew)
         at Microsoft.Build.Experimental.MSBuildClient.TryLaunchServer()
         at Microsoft.Build.Experimental.MSBuildClient.Execute(CancellationToken cancellationToken)
         at Microsoft.Build.CommandLine.MSBuildClientApp.Execute(String[] commandLine, String msbuildLocation, CancellationToken cancellationToken)
         at Microsoft.Build.CommandLine.MSBuildApp.Main(String[] args)
         at Microsoft.DotNet.Cli.Utils.MSBuildForwardingAppWithoutLogging.ExecuteInProc(String[] arguments)
         ```

joperezr · 2022-09-09T22:08:42Z

Yes, I'm working with @tannergooding to see if this is caused by the SDK update.

carlossanlop · 2022-09-10T06:26:51Z

@joperezr merging as requested.

joperezr added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) area-Meta labels Aug 30, 2022

joperezr requested review from jkoritzinsky, CyrusNajmabadi, ViktorHofer and ericstj August 30, 2022 16:29

ghost assigned joperezr Aug 30, 2022

CyrusNajmabadi reviewed Aug 30, 2022

View reviewed changes

Directory.Build.props Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Aug 30, 2022

View reviewed changes

...ntime.InteropServices/gen/LibraryImportGenerator/Analyzers/ConvertToLibraryImportAnalyzer.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Aug 30, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/gen/System.Private.CoreLib.Generators.csproj Show resolved Hide resolved

eerhardt reviewed Aug 30, 2022

View reviewed changes

Directory.Build.props Outdated Show resolved Hide resolved

jkoritzinsky mentioned this pull request Aug 31, 2022

[main] Update dependencies from dotnet/runtime dotnet/sdk#27406

Merged

joperezr force-pushed the UpgradeToRoslyn44 branch from ee7ff53 to 16c7829 Compare September 1, 2022 21:19

joperezr removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 1, 2022

ViktorHofer approved these changes Sep 1, 2022

View reviewed changes

joperezr mentioned this pull request Sep 1, 2022

Updating SDK and compiler versions to RC1 dotnet/arcade#10724

Merged

joperezr mentioned this pull request Sep 8, 2022

[release/7.0] Update dependencies from dotnet/arcade #75220

Closed

joperezr added 2 commits September 9, 2022 10:32

Updating inbox source generators to Roslyn 4.4 and removing polyfill …

1f9df00

…approach

Updating the sdk to match Arcade's version

07bdf7f

joperezr force-pushed the UpgradeToRoslyn44 branch from 16c7829 to 07bdf7f Compare September 9, 2022 17:40

This was referenced Sep 9, 2022

[release/7.0] Update dependencies from dotnet/emsdk #75361

Merged

[release/7.0] Update dependencies from dotnet/llvm-project #75326

Merged

carlossanlop merged commit 94ffb41 into dotnet:release/7.0 Sep 10, 2022

joperezr deleted the UpgradeToRoslyn44 branch September 10, 2022 06:29

This was referenced Sep 12, 2022

Reverting change in source generators that require Roslyn 4.4 until we have a VS version with the compiler fix to consume them. #75490

Merged

Updating inbox source generators to Roslyn 4.4 and removing polyfill approach #75717

Merged

joperezr mentioned this pull request Sep 20, 2022

[release/7.0-rc2] Updating inbox source generators to Roslyn 4.4 and removing polyfill approach #75875

Merged

ghost locked as resolved and limited conversation to collaborators Oct 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Release/7.0] Updating inbox source generators to Roslyn 4.4 and removing polyfill approach #74822

[Release/7.0] Updating inbox source generators to Roslyn 4.4 and removing polyfill approach #74822

joperezr commented Aug 30, 2022 •

edited

Loading

ghost commented Aug 30, 2022

joperezr commented Aug 30, 2022

CyrusNajmabadi commented Aug 30, 2022

carlossanlop commented Aug 30, 2022

joperezr commented Aug 30, 2022

carlossanlop commented Aug 30, 2022

danmoseley commented Aug 30, 2022

ViktorHofer commented Aug 30, 2022 •

edited

Loading

joperezr commented Aug 30, 2022

danmoseley commented Aug 30, 2022

carlossanlop commented Sep 1, 2022

marcpopMSFT commented Sep 1, 2022

joperezr commented Sep 1, 2022

joperezr commented Sep 1, 2022

joperezr commented Sep 1, 2022

carlossanlop commented Sep 2, 2022

danmoseley commented Sep 7, 2022

carlossanlop commented Sep 8, 2022

joperezr commented Sep 8, 2022

joperezr commented Sep 9, 2022

lewing commented Sep 9, 2022

joperezr commented Sep 9, 2022

carlossanlop commented Sep 10, 2022

[Release/7.0] Updating inbox source generators to Roslyn 4.4 and removing polyfill approach #74822

[Release/7.0] Updating inbox source generators to Roslyn 4.4 and removing polyfill approach #74822

Conversation

joperezr commented Aug 30, 2022 • edited Loading

Description

Customer Impact

Regression

Risk

ghost commented Aug 30, 2022

joperezr commented Aug 30, 2022

CyrusNajmabadi commented Aug 30, 2022

carlossanlop commented Aug 30, 2022

joperezr commented Aug 30, 2022

carlossanlop commented Aug 30, 2022

danmoseley commented Aug 30, 2022

ViktorHofer commented Aug 30, 2022 • edited Loading

joperezr commented Aug 30, 2022

danmoseley commented Aug 30, 2022

carlossanlop commented Sep 1, 2022

marcpopMSFT commented Sep 1, 2022

joperezr commented Sep 1, 2022

joperezr commented Sep 1, 2022

joperezr commented Sep 1, 2022

carlossanlop commented Sep 2, 2022

danmoseley commented Sep 7, 2022

carlossanlop commented Sep 8, 2022

joperezr commented Sep 8, 2022

joperezr commented Sep 9, 2022

lewing commented Sep 9, 2022

joperezr commented Sep 9, 2022

carlossanlop commented Sep 10, 2022

joperezr commented Aug 30, 2022 •

edited

Loading

ViktorHofer commented Aug 30, 2022 •

edited

Loading